Skip to content

Fixing Fragment Shader Silent Optimization Issue (Issue #284) #289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zmr-233
Copy link

@zmr-233 zmr-233 commented Jun 19, 2025

Fixing Fragment Shader Silent Optimization Issue (Issue #284)

This PR addresses Issue #284, where the fragment shader successfully compiles but is silently optimized to have no output operations, leading to silent rendering without errors or warnings, making debugging very difficult.

✅ Use --allow-fragment-no-output to optionally skip ERROR

After adding this feature, many of the previous test cases failed in Github CI because they were completely optimized out by DCE (Dead Code Elimination). After analysis, marking fragment shaders with no output as errors indeed leads to unnecessary test failures. Based on the maintainer's suggestion, a compiler flag was added to control this check. You can optionally skip the error by using --allow-fragment-no-output.

Root Cause Analysis

After a thorough analysis of DCE, we found that Issue #284 is actually a "false scenario," based on the following two core facts:

  1. Entry-point functions are never deleted: In the current DCE implementation, all functions listed in the module's OpEntryPoint are unconditionally marked as "root," making them reachable rooted objects. Even if all instructions in the function body are identified as pure calculations and removed, the function definition and OpEntryPoint declaration are retained. DCE does not remove the entire entry-point function.

  2. The "silently compiling out" phenomenon is actually the simplification of the function body: When no "obvious output" is written, DCE removes internal computations (such as OpCompositeConstruct, temporary OpLoad, etc.) and leaves only a direct OpReturn. However, it does not remove %main_fs = OpFunction... or OpEntryPoint Fragment %main_fs "main_fs" .... In the SPIR-V module generated by the compiler, the fragment shader still exists—it just no longer writes to the Output variable.

Thus:

  • The phenomenon in Issue Fragment shader can be silently compiled out #284, where "the entire fragment shader is silently compiled out," is a misunderstanding of the behavior: "function body is simplified to not write outputs" → "no color is written."

  • The initially proposed "silent compile-out" didn’t actually happen: The SPIR-V still contains empty functions and entry-point declarations; it's just that nothing is rendered during the rendering phase.

Solution

Part A — Add Error for Fragment Shaders with "Empty Output"

To fix this, we need to add a check/error for fragment shaders with "empty outputs," so users can immediately know: "Hey, this fragment shader doesn't write anything to the output, it’s definitely a bug," rather than generating an invalid SPIR-V that "renders nothing."

Therefore, I added a validation step after DCE to check for fragment shaders with no output operations:

  • validate_fragment_shader_outputs(): The main validation function called after DCE.
  • fragment_shader_writes_output(): Recursively checks for output operations.
  • has_partial_output_writes(): Detects evidence of partial writes that were optimized away.
  • is_output_storage_variable(): Identifies output variables in different scopes.

Part B — Fix CodegenArgs::from_session Comma-Separated Parameter Parsing Issue

During testing, we also discovered that CodegenArgs was incorrectly parsing comma-separated parameters, such as:

// compile-flags: -C llvm-args=--disassemble-entry=main,--allow-fragment-no-output

So, the code at crates/rustc_codegen_spirv/src/codegen_cx/mod.rs:340 was fixed to:

let expanded_args: Vec<String> = sess.opts.cg.llvm_args
    .iter()
    .flat_map(|arg| arg.split(',').map(|s| s.to_string()))
    .collect();

Part C — Add --allow-fragment-no-output to Optionally Skip ERROR

All relevant test cases involving "no output fragment shaders" have now been marked with this flag. Theoretically, they should pass in Github CI. However, there are still two failing tests locally (which appear unrelated to my changes):

  • ui/spirv-attr/invalid-target.rs: JSON output too large, causing parsing errors (pre-existing issue).
  • ui/spirv-attr/all-builtins.rs: SPIR-V validator strictness issue (environment-related).

Example Error Message

I added a test case that will always fail to verify that silent failures trigger an error message:

  • tests/compiletests/ui/dis/issue-284-dead-fragment.rs: A fragment shader with no output (correctly fails and provides a clear error).

Before the fix (silent failure): The shader compiled successfully but rendered nothing.

After the fix (clear error and guidance):

error: fragment shader `main_fs` produces no output
  |
  = help: fragment shaders must write to output parameters to produce visible results
  = note: use complete assignment like `*out_frag_color = vec4(r, g, b, a)` instead of partial component assignments
  = note: partial component assignments may be optimized away if not all components are written
  = note: to disable this validation (e.g. for testing), use `--allow-fragment-no-output`

Since the validation only runs after DCE, does not impact the hot compilation path, and only checks fragment shaders (not vertex/compute shaders), its performance impact is minimal.

@zmr-233 zmr-233 closed this Jun 19, 2025
@Firestar99
Copy link
Member

@zmr-233 You don't need to close the PR, just update this one and keep pushing to the same branch

@zmr-233 zmr-233 reopened this Jun 19, 2025
@zmr-233 zmr-233 requested a review from Firestar99 June 19, 2025 09:49
@zmr-233
Copy link
Author

zmr-233 commented Jun 19, 2025

I’ve removed the local [profile.dev] codegen-backend = "llvm" change, so that’s no longer affecting CI. The CI failures you see are expected, as they’re caused by the new validation I added validate_fragment_shader_outputs.

Some existing tests are now failing because of the “no output in fragment shader” error.
I set it as an error, but we can switch it to a warning if you prefer. Let me know how you want to handle it :D

@LegNeato
Copy link
Collaborator

Just thinking out loud here, but maybe a compiler flag to turn this off would be useful? We could then set it in the compiletests and users can turn it off if for some reason they need to accept no output (I can't imagine why though?)

@zmr-233 zmr-233 closed this Jun 26, 2025
@zmr-233
Copy link
Author

zmr-233 commented Jun 26, 2025

Just thinking out loud here, but maybe a compiler flag to turn this off would be useful? We could then set it in the compiletests and users can turn it off if for some reason they need to accept no output (I can't imagine why though?)

I've already updated it, and I've added a compiler flag to optionally turn this off. It can now be set in the compiletests, and users can disable it if, for some reason, they need to accept no output (though I can't imagine why : )

@zmr-233 zmr-233 reopened this Jun 26, 2025
zmr-233 added 7 commits June 27, 2025 13:51
Replace deprecated egrep command with the modern equivalent 'grep -E'
to eliminate warnings about egrep being obsolescent. This improves
the lint script output clarity without changing functionality.

Also add codegen-backend override to prevent cranelift conflicts
from parent directory configurations, ensuring rust-gpu builds
with the required LLVM backend.
Addresses issue Rust-GPU#284 where fragment shaders were optimized to have no
output operations, causing silent rendering failures.

The real problem is not DCE eliminating entry points (they are always
rooted), but rather fragment shaders being optimized to have no
meaningful output writes to StorageClass::Output variables.

Implementation:
- Add validation after DCE to detect fragment shaders with no output
- Provide helpful error messages with examples
- Detect partial component writes that may have been optimized away
- Recursively check function calls for output operations

Test case:
- issue-284-dead-fragment.rs: Fragment shader with no output (fails with clear error)

The validation catches cases where developers write partial component
assignments that get optimized away, providing clear guidance on how
to fix the issue.
This addresses maintainer feedback to keep the codegen-backend setting
as a local-only configuration rather than in the committed repository.
Previously, when multiple llvm-args were passed as comma-separated values
(e.g., `-C llvm-args=--disassemble-fn=foo,--allow-fragment-no-output`),
they were treated as a single argument instead of being properly split.

This change modifies CodegenArgs::from_session to:
- Split comma-separated llvm-args into individual arguments
- Preserve existing functionality for single arguments
- Enable proper parsing of multiple compiler flags

Fixes parameter processing for compound llvm-args usage in rust-gpu.
…r validation

Implements a new compiler flag that allows fragment shaders with no output
operations to compile without errors. This addresses issue Rust-GPU#284 where
fragment shaders optimized by DCE (Dead Code Elimination) would fail
validation even when the lack of output was intentional.

Changes:
- Add `allow_fragment_no_output` field to linker Options struct
- Add command-line flag parsing for `--allow-fragment-no-output`
- Skip fragment shader output validation when flag is enabled
- Update error message to include usage hint for the new flag

This flag is intended for testing scenarios and special use cases where
fragment shaders legitimately produce no output. The validation remains
active by default to catch unintentional issues.
Updates 190 fragment shader test files to include the new
--allow-fragment-no-output compiler flag, allowing them to compile
successfully despite producing no output operations.

Changes:
- Add `--allow-fragment-no-output` flag to fragment shader tests
- Handle both new compile-flags lines and comma-separated additions
  to existing llvm-args parameters
- Update corresponding .stderr files to reflect new line numbers
- Preserve issue-284-dead-fragment.rs test to validate error detection

This ensures all existing fragment shader tests pass while maintaining
the ability to detect genuine fragment shader output issues through
the dedicated error test case.
- Fix rustfmt formatting in codegen_cx/mod.rs
- Update expected stderr for sampled_image_rect_query_size_lod_err test
@zmr-233
Copy link
Author

zmr-233 commented Jun 27, 2025

@LegNeato Sorry, I am confident that the branch for this PR can pass the Github CI. Please try to check again. Previously, I kept the origin/main branch and upstream/main in sync with my fork, and I couldn't find a way to manually trigger the Github Action for the origin/pr-zmr233 branch. That's why I have been relying on the PR to trigger CI to identify where the issues were. However, this time I have directly merged origin/main into pr-zmr233, triggered the Github CI, and it has passed : )

@zmr-233 zmr-233 closed this Jun 28, 2025
@zmr-233 zmr-233 reopened this Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants